Skip to content

Fix stale hover reuse for unchanged regions#1533

Open
betamaxbandit wants to merge 1 commit into
eclipse-lsp4e:mainfrom
betamaxbandit:fix1514
Open

Fix stale hover reuse for unchanged regions#1533
betamaxbandit wants to merge 1 commit into
eclipse-lsp4e:mainfrom
betamaxbandit:fix1514

Conversation

@betamaxbandit
Copy link
Copy Markdown

LSPTextHover reused a completed hover request when the viewer and hover region were unchanged. As a result, a repeated hover over the same region could return stale content and avoid issuing a fresh textDocument/hover request.

Fixes #1514

@rubenporras
Copy link
Copy Markdown
Contributor

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

LSPTextHover reused a completed hover request when the viewer and hover
region were unchanged. As a result, a repeated hover over the same
region could return stale content and avoid issuing a fresh
textDocument/hover request.

A previous request is still reused while it is in progress for the same
viewer/region, but any completed requests are now treated as stale, so a
subsequent hover on the same region starts a fresh textDocument/hover
request instead of reusing the old completed result indefinitely.

Fixes eclipse-lsp4e#1514
@betamaxbandit
Copy link
Copy Markdown
Author

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed.
The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

@betamaxbandit
Copy link
Copy Markdown
Author

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

@betamaxbandit
Copy link
Copy Markdown
Author

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

Hi @rubenporras ,
can you help me progress this please?
Cheers John

@FlorianKroiss
Copy link
Copy Markdown
Contributor

I haven't reviewed the code (yet), but I approved the workflow runs

@rubenporras
Copy link
Copy Markdown
Contributor

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed. The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

I think this will trigger many unneeded recomputations. Could we do it so that we invalidate the last computed hover request only if the editor looses focus?

That should cover the case of external resources and be less agressive on the computation side.

@betamaxbandit
Copy link
Copy Markdown
Author

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed. The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.
I've updated the commit message appropriately.
Cheers John

I think this will trigger many unneeded recomputations. Could we do it so that we invalidate the last computed hover request only if the editor looses focus?

That should cover the case of external resources and be less agressive on the computation side.

Hi @rubenporras ,

I like the goal of optimising the number of recomputations, and your idea of triggering this on editor focus lost, but I'm skeptical whether its worth it or always beneficial.

For instance, consider the case where the editor focus is not lost, but an external event causes the need for a recomputation. That could happen if the user starts a project build using a keybaord shortcut (ctrl+B) and this causes header files to be built which generate new values in the external resources.

Also I did some tests in vscode and see that it always generates a hover request without trying to optimise the request - see below.

Would you be willing to reconsider this please?

==
In vscode*, hovering testA twice, genrates 2 identical sets of clangd hover requests.

testA hover 1

V[17:20:34.014] <<< {"id":"clangd-proxy/internal/51","jsonrpc":"2.0","method":"textDocument/definition","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:20:34.014] <-- textDocument/definition("clangd-proxy/internal/51")
V[17:20:34.014] ASTWorker running Definitions on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:20:34.014] --> reply:textDocument/definition("clangd-proxy/internal/51") 0 ms
V[17:20:34.014] >>> {"id":"clangd-proxy/internal/51","jsonrpc":"2.0","result":[{"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}},"uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}]}

I[17:20:34.014] --> textDocument/clangd.fileStatus
V[17:20:34.014] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

V[17:20:34.015] <<< {"id":196,"jsonrpc":"2.0","method":"textDocument/hover","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:20:34.015] <-- textDocument/hover(196)
V[17:20:34.015] ASTWorker running Hover on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:20:34.017] --> reply:textDocument/hover(196) 1 ms
V[17:20:34.017] >>> {"id":196,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"### variable `testA`  \n\n---\nType: `int`  \nValue = `100 (0x64)`  \n\n---\n```cpp\n// In test\nint testA = _A_\n```"},"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}}}}

I[17:20:34.017] --> textDocument/clangd.fileStatus
V[17:20:34.017] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

testA hover 2

V[17:23:52.854] <<< {"id":"clangd-proxy/internal/52","jsonrpc":"2.0","method":"textDocument/definition","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:23:52.854] <-- textDocument/definition("clangd-proxy/internal/52")
V[17:23:52.856] ASTWorker running Definitions on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:23:52.856] --> reply:textDocument/definition("clangd-proxy/internal/52") 2 ms
V[17:23:52.856] >>> {"id":"clangd-proxy/internal/52","jsonrpc":"2.0","result":[{"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}},"uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}]}

I[17:23:52.856] --> textDocument/clangd.fileStatus
V[17:23:52.856] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

V[17:23:52.857] <<< {"id":197,"jsonrpc":"2.0","method":"textDocument/hover","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:23:52.857] <-- textDocument/hover(197)
V[17:23:52.857] ASTWorker running Hover on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:23:52.859] --> reply:textDocument/hover(197) 2 ms
V[17:23:52.859] >>> {"id":197,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"### variable `testA`  \n\n---\nType: `int`  \nValue = `100 (0x64)`  \n\n---\n```cpp\n// In test\nint testA = _A_\n```"},"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}}}}

I[17:23:52.860] --> textDocument/clangd.fileStatus
V[17:23:52.860] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

*: vscode using extension "llvm-vs-code-extensions.vscode-clangd" and settings.json set with:
"clangd.arguments": [
"--log=verbose"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LSPTextHover reuses completed hover result for unchanged region and does not issue a new textDocument/hover request

3 participants